[#9015] improvement(lance-rest): Add UTs and ITs for Lance rest table operations#9016
Conversation
| if (!store.delete(ident, Entity.EntityType.TABLE)) { | ||
| throw new RuntimeException("Failed to drop Lance table: " + ident.name()); | ||
| } |
There was a problem hiding this comment.
The false means "failed to store" or "the table not existed"? If the latter, it should return false, not throwing an exception.
The semantics are misleading; you need to confirm with @mchades .
There was a problem hiding this comment.
Generic lakehouse will return false if the table not existed.
There was a problem hiding this comment.
To be honest, the current code organization scatters the logic for table operations between GenericLakehouseCatalogOperation and LanceCatalogOperation, which reduces code readability (you need to pay attention to both code logics when operating Lance tables) and introduces some repetitive operations (such as repeatedly gettingTable from store), I think we should consider this issue in our future refactoring.
There was a problem hiding this comment.
BTW, why do you call store.get twice?
The code here is confused; both the caller and callee will access the DB, and the callee handles some exceptions, but some rely on the caller.
The lance should only focus on lance-specific logic, whereas the generic lakehouse operation should only focus on the metadata-related CRUD operations.
There was a problem hiding this comment.
To be honest, the current code organization scatters the logic for table operations between
GenericLakehouseCatalogOperationandLanceCatalogOperation, which reduces code readability (you need to pay attention to both code logics when operating Lance tables) and introduces some repetitive operations (such as repeatedly gettingTable from store), I think we should consider this issue in our future refactoring.
I agree, the implementation is not good, it messes with different logic, and doesn't clearly separate the responsibilities.
There was a problem hiding this comment.
I remembered that @youngyjd mentioned that they don't want to access the storage. How do you handle this?
There was a problem hiding this comment.
Dataset.drop lies in the purgeTable operation, and for deregisterTable I use dropTable, which will not remove lance data.
| throw new UnsupportedOperationException( | ||
| "Describing specific table version is not supported."); |
There was a problem hiding this comment.
Which exception and http code will be converted for this UnsupportedOperationException?
There was a problem hiding this comment.
Drop operations will be done in GenericLakehouseCatalogOperations#drop and code will not reach lance#drop.
Another point is that we do not support drop tables in LRS currently.
| entry -> { | ||
| map.put(entry.getKey(), entry.getValue().asText()); | ||
| }); | ||
| return map; |
There was a problem hiding this comment.
You'd better define a helper function for this, and clearly describe why do you implement like this (not leave the comment here).
|
All resolved. |
|
|
||
| @Override | ||
| public DescribeTableResponse describeTable(String tableId, String delimiter) { | ||
| public DescribeTableResponse describeTable( |
There was a problem hiding this comment.
I would suggest you to split this file into small ones according to the different operations. As you continue to add more operations, this file will be too large to maintain.
There was a problem hiding this comment.
Split it into three files
… table operations (apache#9016) ### What changes were proposed in this pull request? Add ITs for lance rest service and test lance table operations. ### Why are the changes needed? To make code robust Fix: apache#9015 ### Does this PR introduce _any_ user-facing change? N/A. ### How was this patch tested? It's self a test.
… table operations (apache#9016) ### What changes were proposed in this pull request? Add ITs for lance rest service and test lance table operations. ### Why are the changes needed? To make code robust Fix: apache#9015 ### Does this PR introduce _any_ user-facing change? N/A. ### How was this patch tested? It's self a test.
… table operations (apache#9016) ### What changes were proposed in this pull request? Add ITs for lance rest service and test lance table operations. ### Why are the changes needed? To make code robust Fix: apache#9015 ### Does this PR introduce _any_ user-facing change? N/A. ### How was this patch tested? It's self a test.
3 participants
What changes were proposed in this pull request?
Add ITs for lance rest service and test lance table operations.
Why are the changes needed?
To make code robust
Fix: #9015
Does this PR introduce any user-facing change?
N/A.
How was this patch tested?
It's self a test.